Feature/conversations by AquaWolf · Pull Request #3313 · element-hq/element-android

AquaWolf · 2021-05-11T07:40:08Z

Pull Request Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Pull request is based on the develop branch
Pull request updates CHANGES.md
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off

Fixes #2734 partially (no bubbles)

Signed-off-by: Philipp Neumann 3daquawolf@gmail.com

Extarys · 2021-05-28T15:15:53Z

Not sure who we can ping to take a look. I'm sure many users would love this feature!

I noticed some errors in the build process, maybe you need to fix those?

AquaWolf · 2021-05-29T06:34:00Z

Yeah there is a typo: workdir/vector/build/generated/resolved/gplayRelease/values-pt/resolved.xml:36: Error: "radio" is a common misspelling; did you mean "rádio" ? [Typos]

But I didn't changed the translations. Can I just change this word or is there some other action required?

I found two bugs:

When logging out and in with another user there is an issue that new conversations are not as priority. I need to clear the list when logging out.
New conversation notifications seems to muted as standard value

I'll try to fix this this weekend or next week.

bmarty · 2021-10-04T20:59:11Z

CHANGES.md

 - Spaces beta

 Improvements 🙌:
+ - Priority conversations for Android 11+ (#2734)


Change on this file is not needed anymore, can you revert please?

bmarty · 2021-10-04T20:59:23Z

newsfragment/3313.feature

@@ -0,0 +1 @@
+Priority conversations for Android 11+


bmarty · 2021-10-04T21:00:25Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt


        // We cannot use NotificationManagerCompat here.
-        val setting = context.getSystemService<NotificationManager>()!!.currentInterruptionFilter
+        val setting = context.getSystemService(NotificationManager::class.java)!!.currentInterruptionFilter


Is this change necessary?

Not necessary but fixes some linting for me locally should i revert?

What is the lint issue you are seeing? I do not see it, nor the CI.

Ok i will revert this.

bmarty

@AquaWolf thanks for the PR. Did you try to fix the 2 issues you've found?

AquaWolf · 2021-10-05T05:17:29Z

@AquaWolf thanks for the PR. Did you try to fix the 2 issues you've found?

The mute thing didn't appeared anymore.

AquaWolf · 2021-10-05T05:27:09Z

because of "shortcut" handling when logging out, there is a method removeAllDynamicShortcuts
actually I don't know where the logout handling is done, this PR is now a bit longer away.
My thoughts the shortcuts: There should be a more specific handling regarding the notification settings in the app (All/Keywords Metions/None) The none and mentions shouldn't be pushed to the shortcut list.
@bmarty I would write you in Matrix perhaps you can give me some hint where the logout handling is done :)

bmarty · 2021-10-05T07:39:12Z

because of "shortcut" handling when logging out, there is a method removeAllDynamicShortcuts actually I don't know where the logout handling is done, this PR is now a bit longer away. My thoughts the shortcuts: There should be a more specific handling regarding the notification settings in the app (All/Keywords Metions/None) The none and mentions shouldn't be pushed to the shortcut list. @bmarty I would write you in Matrix perhaps you can give me some hint where the logout handling is done :)

The method removeAllDynamicShortcuts is already called during log out from this method: https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/home/ShortcutsHandler.kt#L72

Also the CHANGES.md file is not OK. Is it OK for you to rebase the PR so we can have a clean history without any change on this file?

bmarty · 2021-10-05T07:40:40Z

vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt


        // We cannot use NotificationManagerCompat here.
-        val setting = context.getSystemService<NotificationManager>()!!.currentInterruptionFilter
+        val setting = (context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager).currentInterruptionFilter


Why not fully revert this change?

I get a error while compiling letting it as is

will revert this

AquaWolf · 2021-10-05T11:24:45Z

rebased and force pushed on my remote hope it's now better organized and implemented a function to remove all longlived shortcuts when logging out 👍

bmarty

Thanks for the update. I have one more remark, sorry I did not see it in the beginning.

bmarty · 2021-10-05T12:17:52Z

vector/src/main/java/im/vector/app/features/notifications/NotificationDrawerManager.kt

+                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {
+                        val openRoomIntent = RoomDetailActivity.shortcutIntent(context, roomId)
+
+                        val shortcut = ShortcutInfoCompat.Builder(context, roomId)


Move this code to ShortcutCreator?

We will handle it. Thanks again for the PR!

dkter · 2021-11-04T15:00:55Z

Conversation features have already been supported since #1809? Not sure what this accomplished but it looks like the room avatar cropping bug that I fixed in 5df0bd6 has been reverted.

Extarys · 2021-11-04T17:33:52Z

@dkter According to #1809 (comment) it doesn't support the bubble. This current PR implements it. (From what I can understand)

dkter · 2021-11-04T17:50:13Z

@dkter According to #1809 (comment) it doesn't support the bubble. This current PR implements it. (From what I can understand)

@Extarys This PR doesn't implement bubbles (see the top post). It seems like it just replaces removeAllDynamicShortcuts with removeLongLivedShortcuts, and duplicates some of the code that's already in ShortcutCreator in NotificationDrawerManager (albeit not using Bitmap.toProfileImageIcon, which should be used to render the room icons correctly).

What seems to be happening now is that shortcuts are sometimes created with the code in ShortcutCreator, and thus have correctly rendered icons, and sometimes are created with the code in NotificationDrawerManager which gives them broken icons.

It looks like there's been a major refactoring of the notification code since this PR, but the redundant code still exists.

bmarty reviewed Oct 4, 2021

View reviewed changes

newsfragment/3313.feature

@@ -0,0 +1 @@

Priority conversations for Android 11+

Copy link

Member

bmarty Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

bmarty reviewed Oct 4, 2021

View reviewed changes

bmarty reviewed Oct 5, 2021

View reviewed changes

AquaWolf force-pushed the feature/conversations branch from 26159d0 to 5bfa050 Compare October 5, 2021 11:09

bmarty reviewed Oct 5, 2021

View reviewed changes

VWPhilipp added 3 commits October 5, 2021 14:59

added dynamic shortcut for priority conversations for Android 11+

214deaa

remove also LongLivedShortcuts

95247f8

fixed ordering and url linting error

2223e95

AquaWolf force-pushed the feature/conversations branch from 89e82b9 to 2223e95 Compare October 5, 2021 13:02

bmarty approved these changes Oct 13, 2021

View reviewed changes

bmarty enabled auto-merge October 13, 2021 09:02

bmarty merged commit 4f51dbd into element-hq:develop Oct 13, 2021

dkter mentioned this pull request Nov 5, 2021

Conversation-style icons are cropped incorrectly #4417

Closed

ouchadam mentioned this pull request Nov 8, 2021

Fix incorrect cropping of conversation icons #4424

Merged

9 tasks

AmineI mentioned this pull request Apr 4, 2023

Fully support Android 11 Conversations #1722

Open

5 tasks

Comments

Conversation

AquaWolf commented May 11, 2021

Pull Request Checklist

Fixes #2734 partially (no bubbles)

Uh oh!

Extarys commented May 28, 2021

Uh oh!

AquaWolf commented May 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

AquaWolf commented Oct 5, 2021

Uh oh!

AquaWolf commented Oct 5, 2021

Uh oh!

bmarty commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AquaWolf commented Oct 5, 2021

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkter commented Nov 4, 2021

Uh oh!

Extarys commented Nov 4, 2021

Uh oh!

dkter commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dkter commented Nov 4, 2021 •

edited

Loading